Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

エントリーポイントの確認 #1025

Merged
merged 4 commits into from
Oct 27, 2024
Merged

エントリーポイントの確認 #1025

merged 4 commits into from
Oct 27, 2024

Conversation

ikura-hamu
Copy link
Member

@ikura-hamu ikura-hamu commented Oct 27, 2024 •
edited by github-actions bot
Loading

User description

fix: #1016

  • ✨ serviceでzipのエントリーポイントをチェックする処理を追加
  • ✨ handlerでzipファイル関係のエラーハンドリング

entryPointがzipファイル内の存在するファイルを指しているかを確認する。この実装の結果、zipファイル以外を弾くようになった。

PR Type

Enhancement, Tests

Description

  • zipファイルとエントリーポイントの検証を追加し、無効なファイルを拒否する機能を実装。
  • 新しいエラーハンドリングを追加し、具体的なエラーメッセージを提供。
  • テストケースを追加し、新しい検証ロジックを検証。
  • APIドキュメントを更新し、新しいエラーハンドリングを反映。

Changes walkthrough 📝

Relevant files
Error handling
game_file.go
エラーハンドリングの強化とエラーメッセージの改善                                                                 

src/handler/v2/game_file.go

  • エラーハンドリングを追加し、無効なエントリーポイントやzipファイル以外のファイルを拒否。
  • エラーメッセージをより具体的に修正。
 +7/-1     
Tests
game_file_test.go
エラーハンドリングのテストケース追加                                                                             

src/handler/v2/game_file_test.go

  • 新しいテストケースを追加し、エラーハンドリングを検証。
  • zipファイルとエントリーポイントの検証をテスト。
 +20/-0   
game_file_test.go
zipファイルとエントリーポイント検証のテスト追加                                                               

src/service/v2/game_file_test.go

  • 新しいテスト関数を追加し、zipファイルとエントリーポイントの検証をテスト。
 +168/-28
Enhancement
errors.go
新しいエラー定数の追加                                                                                           

src/service/errors.go

  • 新しいエラー定数を追加。
 +2/-0     
game_file.go
zipファイルとエントリーポイントの検証機能追加                                                                 

src/service/v2/game_file.go

  • zipファイルとエントリーポイントの検証ロジックを追加。
  • 一時ファイルを使用してzipファイルを検証。
 +68/-1   
Documentation
v2_game_file.go
関数コメントの更新                                                                                               

src/service/v2_game_file.go

  • 関数のコメントを更新し、新しいエラーハンドリングを反映。
 +2/-0     
v2.yaml
APIドキュメントの更新                                                                                         

docs/openapi/v2.yaml

  • APIバージョンを更新。
  • エラーメッセージの説明を追加。
 +2/-1     

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

@github-actions GitHub Actions
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

1016 - Partially compliant

Fully compliant requirements:

  • エントリーポイントのバリデーションを追加する。
  • zipファイル内に指定のパスが存在するか確認する。

Not compliant requirements:

  • 実行ファイルの形式のチェックを行う。
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Error Handling
エラーハンドリングのロジックが複雑で、エラーの原因を特定しにくい可能性がある。

Error Message Consistency
エラーメッセージの一貫性がなく、ユーザーが混乱する可能性がある。

@github-actions GitHub Actions
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Enhancement
一時ファイルの削除処理を改善して、不要なファイルが残らないようにする。

checkZip関数内で一時ファイルを作成後、コピーが完了する前にエラーが発生した場合、ファイルが削除されない可能性があります。defer
os.Remove(f.Name())をio.Copyの後に移動して、ファイルが正常にコピーされた後にのみ削除するようにしてください。

src/service/v2/game_file.go [44-52]

 f, err := os.CreateTemp("", "game_file")
 if err != nil {
     return nil, false, fmt.Errorf("failed to create temp file: %w", err)
 }
+_, err = io.Copy(f, reader)
+if err != nil {
+    f.Close()
+    os.Remove(f.Name())
+    return nil, false, fmt.Errorf("failed to copy file: %w", err)
+}
 defer os.Remove(f.Name())
-_, err = io.Copy(f, reader)
Suggestion importance[1-10]: 9

Why: The suggestion addresses a potential resource leak by ensuring temporary files are properly deleted even if an error occurs during the copy process. This enhances the robustness and reliability of the file handling logic, preventing unnecessary file accumulation.

9
エラーメッセージの一貫性を向上させる。

エラーメッセージの一貫性を保つために、ErrNotZipFileのエラーメッセージを修正してください。現在のメッセージは「only zip file is
allowed」ですが、他のエラーメッセージと同様に「not a zip file」とすることを提案します。

src/handler/v2/game_file.go [111]

-return echo.NewHTTPError(http.StatusBadRequest, "only zip file is allowed")
+return echo.NewHTTPError(http.StatusBadRequest, "not a zip file")
Suggestion importance[1-10]: 7

Why: The suggestion improves the consistency of error messages by aligning the wording with other similar messages. This enhances readability and maintainability, making it easier for developers to understand and manage error handling.

7
Possible issue
テストケースの説明を正確にする。


テストケースの説明が誤っています。「SaveGameFileがErrNotZipFileなので404」となっていますが、ステータスコードは400です。説明を「SaveGameFileがErrNotZipFileなので400」と修正してください。

src/handler/v2/game_file_test.go [399]

-description:         "SaveGameFileがErrNotZipFileなので404"
+description:         "SaveGameFileがErrNotZipFileなので400"
Suggestion importance[1-10]: 8

Why: Correcting the test case description to reflect the actual status code improves the accuracy and reliability of the test documentation. This is crucial for maintaining clarity and ensuring tests are correctly interpreted and executed.

8

@codecov Codecov
Copy link

codecov bot commented Oct 27, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 71.87500% with 18 lines in your changes missing coverage. Please review.

Project coverage is 49.23%. Comparing base (5cc251f) to head (f7a0f1c).
Report is 20 commits behind head on main.

Files with missing lines Patch % Lines
src/service/v2/game_file.go 68.42% 12 Missing and 6 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1025      +/-   ##
==========================================
+ Coverage   49.10%   49.23%   +0.12%     
==========================================
  Files         121      121              
  Lines       10742    10804      +62     
==========================================
+ Hits         5275     5319      +44     
- Misses       5140     5152      +12     
- Partials      327      333       +6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ikura-hamu ikura-hamu merged commit 7c5b62e into main Oct 27, 2024
10 checks passed
@ikura-hamu ikura-hamu deleted the feat/entrypoint_validate branch October 27, 2024 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Review effort [1-5]: 3
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

エントリーポイントのバリデーション
1 participant
@ikura-hamu